-
Notifications
You must be signed in to change notification settings - Fork 39
React 이관현 sprint5 #185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
React 이관현 sprint5 #185
The head ref may contain hidden characters: "React-\uC774\uAD00\uD604-sprint5"
Conversation
addiescode-sj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
첫 리액트 미션 수고하셨습니다!
폴더 구조, 컨벤션 등 여러가지 참고할만한 코멘트와 아티클을 드려봤어요.
꾸준히 미션을 통해 코드 작성해보시고 리뷰 받아보면서 리팩토링하다보면 지금보다 훨씬 수월하게 코드 작성하실 수 있을거예요 👍
주요 리뷰 포인트
- 페이지네이션 로직 분리 피드백
- 폴더 구조 피드백
- CSS 클래스이름 충돌 방지
- React Router 사용 피드백
| const [totalPages, setTotalPages] = useState(1); | ||
| const pageSize = 10; // 표시할 상품 개수 | ||
| const maxVisible = 5; | ||
|
|
||
| useEffect(() => { | ||
| const fetchTotalPages = async () => { | ||
| try { | ||
| const res = await fetch( | ||
| `https://panda-market-api.vercel.app/products?page=1&pageSize=${pageSize}&orderBy=recent` | ||
| ); | ||
| const data = await res.json(); | ||
| const count = data.totalCount || 1; //보호 처리 | ||
| setTotalPages(Math.ceil(count / pageSize)); | ||
| } catch (err) { | ||
| console.error("페이지 수 불러오기 실패:", err); | ||
| } | ||
| }; | ||
|
|
||
| fetchTotalPages(); | ||
| }, []); | ||
|
|
||
| const currentGroup = Math.floor((currentPage - 1) / maxVisible); | ||
| const maxGroup = Math.floor((totalPages - 1) / maxVisible); | ||
| const startPage = currentGroup * maxVisible + 1; | ||
| const endPage = Math.min(startPage + maxVisible - 1, totalPages); | ||
|
|
||
| const handleClick = (page) => { | ||
| if (page >= 1 && page <= totalPages) { | ||
| setCurrentPage(page); | ||
| } | ||
| }; | ||
|
|
||
| const handlePrevGroup = () => { | ||
| const prevStart = (currentGroup - 1) * maxVisible + 1; | ||
| if (currentGroup > 0) setCurrentPage(prevStart); | ||
| }; | ||
|
|
||
| const handleNextGroup = () => { | ||
| const nextStart = (currentGroup + 1) * maxVisible + 1; | ||
| if (currentGroup < maxGroup) setCurrentPage(nextStart); | ||
| }; | ||
|
|
||
| const pages = []; | ||
| for (let i = startPage; i <= endPage; i++) { | ||
| pages.push(i); // 현재 그룹에 해당하는 페이지만 저장 | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 컴포넌트는 Footer보단 Pagination에 가까워보이네요! 네이밍을 바꿔주세요. 그리고 UI를 재사용할뿐만 아니라 로직만 재사용할 일이 있다면 이런 로직들을 커스텀 훅 단위로 분리해주시면 도움이 되겠죠? :)
| import "./Header.css"; | ||
|
|
||
| const Header = () => { | ||
| const isMarketPage = window.location.pathname === "/items"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 Header 컴포넌트에서 window.location.pathname을 직접 체크하고있는데, 이는 React의 선언적 방식과 맞지 않습니다.
React Router의 NavLink를 사용해서, 현재 경로와 일치할때 클래스이름이 바뀌게끔 개선해보면 어떨까요?
예시)
- React Router를 사용해 라우팅 구조화한 App.jsx
import { BrowserRouter as Router, Routes, Route } from 'react-router-dom';
import Header from "./components/Header/Header";
import Main from "./components/Main/Main";
import Container from "./components/Container/Container";
function App() {
return (
<Router>
<Container>
<Header />
<Routes>
<Route path="/" element={<Main />} />
<Route path="/items" element={<Main />} />
{/* 다른 라우트들도 추가 */}
</Routes>
</Container>
</Router>
);
}
export default App;- Header.jsx
import { NavLink } from 'react-router-dom';
import pandaMarket from "../../images/PandaMarket.png";
import UserLogo from "../../images/UserLogo.svg";
import "./Header.css";
const Header = () => {
return (
<div className="Header">
<NavLink to="/items">
<img
className="Header__logo"
src={pandaMarket}
alt="판마다켓 홈페이지 로고"
/>
</NavLink>
<div className="Header__menu">
<NavLink
className={({ isActive }) =>
`Header__menu__items ${isActive ? "active" : ""}`
}
to="/board"
>
자유게시판
</NavLink>
<NavLink
className={({ isActive }) =>
`Header__menu__items ${isActive ? "active" : ""}`
}
to="/items"
>
중고마켓
</NavLink>
</div>
<NavLink to="/items">
<img className="Header__user" src={UserLogo} alt="유저 아이콘" />
</NavLink>
</div>
);
};
export default Header;NavLink는 현재 경로와 일치할 때 isActive Prop이 true로 바뀌는데, 이를 활용해 active 클래스를 추가하면 될 것 같습니다.
| import Footer from "../Footer/Footer"; | ||
|
|
||
| const Main = () => { | ||
| const [currentPage, setCurrentPage] = useState(1); //페이지 번호 상태 관리 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
페이지네이션 관련 상태 관리 로직이 분산되어있어요.
위 코멘트에서 얘기했던것처럼, 페이지네이션 로직을 커스텀 훅으로 분리하고, 페이지네이션 컴포넌트를 더 재사용 가능하게 개선해주면 이런 상태 관리 로직또한 여기저기서 작성될 필요가 없어질거예요.
이런 구조는 어떨까요?
예시)
- 공통 페이지네이션 로직을 재사용하기위한 커스텀 훅 만들기
import { useState, useEffect } from 'react';
const usePagination = (pageSize = 10, maxVisible = 5) => {
const [currentPage, setCurrentPage] = useState(1);
const [totalPages, setTotalPages] = useState(1);
useEffect(() => {
const fetchTotalPages = async () => {
try {
const res = await fetch(
`https://panda-market-api.vercel.app/products?page=1&pageSize=${pageSize}&orderBy=recent`
);
const data = await res.json();
const count = data.totalCount || 1;
setTotalPages(Math.ceil(count / pageSize));
} catch (err) {
console.error("페이지 수 불러오기 실패:", err);
}
};
fetchTotalPages();
}, [pageSize]);
const currentGroup = Math.floor((currentPage - 1) / maxVisible);
const maxGroup = Math.floor((totalPages - 1) / maxVisible);
const startPage = currentGroup * maxVisible + 1;
const endPage = Math.min(startPage + maxVisible - 1, totalPages);
const handlePageChange = (page) => {
if (page >= 1 && page <= totalPages) {
setCurrentPage(page);
}
};
const handlePrevGroup = () => {
const prevStart = (currentGroup - 1) * maxVisible + 1;
if (currentGroup > 0) setCurrentPage(prevStart);
};
const handleNextGroup = () => {
const nextStart = (currentGroup + 1) * maxVisible + 1;
if (currentGroup < maxGroup) setCurrentPage(nextStart);
};
const pages = Array.from(
{ length: endPage - startPage + 1 },
(_, i) => startPage + i
);
return {
currentPage,
totalPages,
pages,
handlePageChange,
handlePrevGroup,
handleNextGroup,
isFirstGroup: currentGroup === 0,
isLastGroup: currentGroup === maxGroup,
};
};
export default usePagination;- 지금처럼 페이지네이션 처리가 필요할때 해당 훅을 사용해서 값을 만들어주고, Footer 컴포넌트는 커스텀 훅을 사용해 조합한 결과값을 가지고 단순히 보여주는 역할만 (UI) 담당하게 해주기
import usePagination from "../../hooks/usePagination";
import Footer from "../Footer/Footer";
import AllProducts from "./Products/AllProducts";
import BestProducts from "./Products/BestProducts";
const Main = () => {
const {
currentPage,
totalPages,
pages,
handlePageChange,
handlePrevGroup,
handleNextGroup,
isFirstGroup,
isLastGroup,
} = usePagination();
return (
<>
<BestProducts />
<AllProducts currentPage={currentPage} />
<Footer
currentPage={currentPage}
totalPages={totalPages}
pages={pages}
onPageChange={handlePageChange}
onPrevGroup={handlePrevGroup}
onNextGroup={handleNextGroup}
isFirstGroup={isFirstGroup}
isLastGroup={isLastGroup}
/>
</>
);
};
export default Main;- 마지막으로, 기존 Footer 컴포넌트 역할 축소
import "./Footer.css";
const PaginationButton = ({ onClick, disabled, children, className = "" }) => (
<button
className={`footer__btn ${className}`}
onClick={onClick}
disabled={disabled}
>
{children}
</button>
);
const Footer = ({
currentPage,
totalPages,
pages,
onPageChange,
onPrevGroup,
onNextGroup,
isFirstGroup,
isLastGroup,
}) => {
return (
<div className="footer">
<PaginationButton onClick={onPrevGroup} disabled={isFirstGroup}>
<
</PaginationButton>
{pages.map((page) => (
<PaginationButton
key={page}
onClick={() => onPageChange(page)}
className={page === currentPage ? "active" : ""}
>
{page}
</PaginationButton>
))}
<PaginationButton onClick={onNextGroup} disabled={isLastGroup}>
>
</PaginationButton>
</div>
);
};
export default Footer;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
개선사항에 따른 장점
-
관심사 분리: UI와 비즈니스 로직을 명확히 구분 => 페이지네이션 로직만 필요할때는 커스텀훅으로 결과값만 조합할 수 있으니 페이지네이션 관련해서 다른 UI를 써줄때에도 쉽게 변경 및 재사용 가능
-
유지보수성 개선: 각 컴포넌트의 역할이 명확해지고, 코드 중복이 제거되고, 테스트하기 쉬운 구조로 변경됨
| const res = await fetch( | ||
| `https://panda-market-api.vercel.app/products?page=${currentPage}&pageSize=10&orderBy=${sortType}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런 api base url은 노출 위험도 있고, 작성하다가 오타가 날수도있으니 환경 변수를 만들어서 관리해볼까요?
| <div className="AllProducts"> | ||
| <div className="AllProducts__nav"> | ||
| <h3 className="AllProducts__title">전체 상품</h3> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금 페이지이름을 포함해 클래스이름을 지어주고있는데, 이럴 필요 없이 css modules를 사용하면 고유한 클래스이름을 생성해주기때문에, 클래스이름 충돌 문제가 해결될거예요.
아래 아티클 참고해보시고 관현님이 시도하고싶은 방법을 골라서 클래스이름 충돌 문제를 방지해볼까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src 바로 밑에 components 폴더는 일반적으로 여러 페이지에서 공통으로 쓰일만한 컴포넌트를 모아두는 용도로 활용된답니다.
폴더 구조도 바꿔보면 좋을것같아요!
질문에 대한 답변
코드 작성 + 리뷰 + 리팩토링을 습관화하시다보면 저절로 구조화 능력, 유지보수를 고려해 개발하는 습관 등 코드를 작성하며 필요한 여러가지 개발 역량이 좋아질겁니다. 처음부터 잘하는 사람은 없으니까요! 파이팅입니다 👍 관련해서 상세한 피드백 본문 내에 작성해드렸어요! |
요구사항
기본
중고마켓
중고마켓 반응형
베스트 상품
Desktop : 4개 보이기
Tablet : 2개 보이기
Mobile : 1개 보이기
전체 상품
Desktop : 12개 보이기
Tablet : 6개 보이기
Mobile : 4개 보이기
심화
스크린샷
멘토에게